Skip to content

Fix empty array edge cases in nlinfit - #485

Merged
pr0m1th3as merged 9 commits into
gnu-octave:mainfrom
Sonu0305:fix-nlinfit-empty
Sep 16, 2026
Merged

pr0m1th3as merged 9 commits into
gnu-octave:mainfrom
Sonu0305:fix-nlinfit-empty

Conversation

@Sonu0305

Copy link
Copy Markdown
Contributor

No description provided.

@pr0m1th3as

Copy link
Copy Markdown
Member

Matches MATLAB, which errors in all these cases. Remove the %!test block that holds only a comment (it always passes), give the %!error tests the full message with the nlinfit: prefix, and start the message after the prefix in lower case, e.g. "nlinfit: MODELFUN must return a vector of the same size as Y."

@Sonu0305

Copy link
Copy Markdown
Contributor Author

updated @pr0m1th3as
can you review again, thank you.

@pr0m1th3as

Copy link
Copy Markdown
Member

Please remove the rows (X) != rows (y) check. X is only passed to MODELFUN, so it can be anything the model function accepts; on main both of these fit, and with this change both error:

y = [2.1; 3.9; 6.2; 7.8; 10.1];
nlinfit ([], y, @(b, x) b(1) * (1:5)', 1)
nlinfit ({1:5}, y, @(b, x) b(1) * x{1}(:), 1)

MATLAB has no such check either; the size check on the output of MODELFUN is enough. Argument names keep their case (lower case is only for an ordinary first word after the prefix): nlinfit: MODELFUN must return a vector of the same size as Y. Update the %!error tests to the full messages and write zeros (0, 3) with spaces.

@Sonu0305

Copy link
Copy Markdown
Contributor Author

updated @pr0m1th3as

@pr0m1th3as

Copy link
Copy Markdown
Member

nlinfit ((1:6), (1:6), @(b, x) b(1) * x, 1) error with your PR, whereas it passes on main. Stop feeding my replies to an LLM-backed coding assistant and keep committing code that you haven't tested/checked properly.

@Sonu0305

Copy link
Copy Markdown
Contributor Author

fixing it, behaviour changed when i modified the if condition

@Sonu0305

Copy link
Copy Markdown
Contributor Author

can you review now @pr0m1th3as thank you.

@pr0m1th3as
pr0m1th3as merged commit b4887bc into gnu-octave:main Sep 16, 2026
@Sonu0305

Copy link
Copy Markdown
Contributor Author

Thanks for your reviews, @pr0m1th3as .

@Sonu0305
Sonu0305 deleted the fix-nlinfit-empty branch September 16, 2026 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants